-
Couldn't load subscription status.
- Fork 75
[FLINK-38545] Update to Flink 1.20.3 and Pulsar 3.0.5 #111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html) |
621d6d4 to
7dd5769
Compare
a5765ca to
536ec46
Compare
a0beef9 to
8e071f5
Compare
8e071f5 to
c956513
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just added one nit. Thanks.
| */ | ||
| public int getFetchOneMessageTime() { | ||
| return fetchOneMessageTime; | ||
| return (int) fetchOneMessageTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The fetchOneMessageTime field is stored as long but cast to int in the getter, can it risk overflow for large Duration values? Should we add: Add validation to reject values > Integer.MAX_VALUE , or can we make this Duration too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Cannot do much about this, as the pulsar client requires an int value for this, and Duration.toMillis() return a long. I don't think there can be any case, where someone types in a duration that turns into a bigger number than Integer.MAX_VALUE. Since I can type in durations like 5d, having an explicit check that says you need to type in a lower number than 2147483647 would be more helpful than the cast exception.
Well, we could leave this as an int, but it is much better UX to be able to write duration strings than raw millis, so I would consider the current state better then it was before.
|
Awesome work, congrats on your first merged pull request! |
Key changes
3h,2metc), so made duration-like configs like that where it made sense